-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Android & iOS] dialog theme change on changing UserAppTheme #24559
Conversation
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @jfversluis ?
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@@ -83,5 +86,9 @@ protected override PlatformView CreatePlatformElement() => | |||
/// <param name="application">The associated <see cref="IApplication"/> instance.</param> | |||
/// <param name="args">The associated command arguments.</param> | |||
public static partial void MapCloseWindow(ApplicationHandler handler, IApplication application, object? args); | |||
|
|||
#if ANDROID || IOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it just needed for IOS or even for Mac Catalyst? I'm not sure if IOS
means IOS + MacCatalyst or not :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MartyIX Glad you're here :) Could you maybe have a look at how Windows behaves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR is about "changing app theme after an app is started", then WiUI 3 unfortunately does not support this nicely. Feel free to upvote this issue.
What can be done is: microsoft/microsoft-ui-xaml#4474 (comment) to modify elements that are in the visual tree (like this) and then add some custom mappings to make sure that elements that are not in the UI tree are switched to correct theme as they are inserted in the UI tree. I should create a sample for this because it's quite nuanced.
-> A lot of sadness here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I spent a bit of time to put together this: #21042 (comment). It's "working solution" but it's not exactly nice because the API is limited.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the theme not flow to the children? So it is not possible to set the theme on the window or the root content and get magic to flow?
I know in Maui itself, we use the dynamic resource system to push the theme to each child. But, I suppose if we had to we can maybe do the same on windows.
We already have a hack to toggle the theme on and off just to get dynamic colors to update. If there is a perf issue, we can push back and say the WinUI team has to either fix or provide some other way to set the theme like the rest of the development world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattleibow WinUI behavior is explained very nicely here:
Proposal would allow to change theme on runtime (which would be the best really).
Does the theme not flow to the children? So it is not possible to set the theme on the window or the root content and get magic to flow?
No. I don't believe so.
If there is a perf issue, we can push back and say the WinUI team has to either fix or provide some other way to set the theme like the rest of the development world.
Well, if one is forced to switch theme one control at a time, then for a big app, it might take a long-ish time. However, it might still be acceptable because one does not change themes often. But yes, it seems that WinUI has all necessary plumbing in place and supporting app theme changes should be "relatively easy" for them to implement given that they support: User changes theme in Windows settings and app re-renders correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, maybe for another PR we try to see if the theme flows, and if not, we see how long it really takes to update all the children's themes. Maybe it is super fast? I think that when we "refresh the theme resources" with our hack that "sets the theme to unspecified and then back" for things like button backgrounds, it is so fast that the UI did not have time to notice the change. I think this is because it is sort of setting a flag for the next UI cycle, so if we update all the controls, it may not actually trigger a direct refresh but wait for the next UI "cycle" or "frame".
Description of Change
Dialog theme didn't change on changing UserAppTheme. It works when changing the app theme via the system's settings thanks to this PR: #13318
Issues Fixed
Fixes #24558
Screen.Recording.2024-08-31.at.23.41.51.mov
Screen.Recording.2024-09-01.at.00.04.39.mov